Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go_test: Allow not registering a SIGTERM handler #3827

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

illicitonion
Copy link
Contributor

What type of PR is this?

Bug fix
Feature

What does this PR do? Why is it needed?

The rules_go SIGTERM handler causes tests which themselves test
responding to SIGTERM to panic and fail.

@illicitonion illicitonion force-pushed the optional-sigterm-handler branch 2 times, most recently from 98260b7 to f808fd7 Compare January 12, 2024 17:38
Copy link
Member

@fmeum fmeum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cc @linzhp Would this help with your tests?

go/private/rules/test.bzl Outdated Show resolved Hide resolved
@linzhp
Copy link
Contributor

linzhp commented Jan 12, 2024

This helps, but we simply add signal.Reset(syscall.SIGTERM) to the test cases or TestMain of them that register SIGTERM handler. @illicitonion did you try that approach?

The rules_go SIGTERM handler causes tests which themselves test
responding to SIGTERM to panic and fail.
@illicitonion
Copy link
Contributor Author

This helps, but we simply add signal.Reset(syscall.SIGTERM) to the test cases or TestMain of them that register SIGTERM handler. @illicitonion did you try that approach?

Ooh that's a nice idea! Yeah, this approach works, and it seems reasonable (though a little noisy) for tests which are explicitly handling signals themselves, but it feels much noisier to have to add to all tests if they use goleak...

@fmeum
Copy link
Member

fmeum commented Jan 15, 2024

goleak will come with a default ignorelist entry in the future as far as I understood @linzhp.

I do prefer the Go-only solution in that case if we document it.

@linzhp
Copy link
Contributor

linzhp commented Jan 15, 2024

Good point on goleak. Uber maintains a internal patch of all known leaks, but there is no plan to put the list in the open source goleak project. It's unfair to ask every user of goleak to maintain a patch. So in combination of the SIGTERM reset and goleak opt-out, this PR is quite useful, unless we have better implementation of the timeout handler.

@fmeum
Copy link
Member

fmeum commented Jan 16, 2024

The more I think about it, the less I think this new attribute provides a good user experience: It's used to work around two separate issues, both of which would better be fixed in other layers.

For the few tests that require bespoke SIGTERM handling, the solution of manually resetting the handler from Go seems much more targeted and discoverable. It also communicates the intent better and guards against other conflicting handlers other libraries may register in init.

For goleak, disabling the new timeout handler just to avoid the false positive is also not great as it means users have to choose between using goleak and having better timeout reporting.

@linzhp Would goleak be open to contributions to improve this interaction?

@linzhp
Copy link
Contributor

linzhp commented Jan 16, 2024

I am not a maintainer of goleak, based on this reply, they don't want a default ignore list, given that goleak supports IgnoreAnyFunction.

So tests that uses goleak can call VerifyNone(t, goleak.IgnoreAnyFunction("github.com/bazelbuild/rules_go/go/tools/bzltestutil.RegisterTimeoutHandler.func1"))) or

func TestMain(m *testing.M) {
  goleak.VerifyTestMain(m, goleak.IgnoreAnyFunction("github.com/bazelbuild/rules_go/go/tools/bzltestutil.RegisterTimeoutHandler.func1"))
}

malt3 added a commit to edgelesssys/constellation that referenced this pull request Jan 16, 2024
rules_go added a SIGTERM handler that has a goroutine that survives the scope of the goleak check.
Currently, the best known workaround is to ignore this goroutine.

uber-go/goleak#119
bazel-contrib/rules_go#3749
bazel-contrib/rules_go#3827 (comment)
malt3 added a commit to edgelesssys/constellation that referenced this pull request Jan 16, 2024
rules_go added a SIGTERM handler that has a goroutine that survives the scope of the goleak check.
Currently, the best known workaround is to ignore this goroutine.

uber-go/goleak#119
bazel-contrib/rules_go#3749
bazel-contrib/rules_go#3827 (comment)
malt3 added a commit to edgelesssys/constellation that referenced this pull request Jan 16, 2024
rules_go added a SIGTERM handler that has a goroutine that survives the scope of the goleak check.
Currently, the best known workaround is to ignore this goroutine.

uber-go/goleak#119
bazel-contrib/rules_go#3749
bazel-contrib/rules_go#3827 (comment)
@fmeum
Copy link
Member

fmeum commented Jan 16, 2024

I followed up on the goleak thread, maybe we can solve this by introducing an ignore mechanism that rules_go can use and goleak doesn't have to maintain.

malt3 added a commit to edgelesssys/constellation that referenced this pull request Jan 22, 2024
rules_go added a SIGTERM handler that has a goroutine that survives the scope of the goleak check.
Currently, the best known workaround is to ignore this goroutine.

uber-go/goleak#119
bazel-contrib/rules_go#3749
bazel-contrib/rules_go#3827 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants